Wrap SassC/Sass gems #108

landongrindheim · 2019-09-04T12:40:44Z

This is in response to a bug which was pointed out in
#106. We added require 'sassc',
which turned out to be a breaking change. If a user doesn't have sassc
listed in their Gemfile, they'll get load errors.

In order to resolve this, I'm opting to wrap both SassC and Sass,
with the idea that we can remove the reliance on Sass soon (the
sass-ruby gem is past end-of-life).

This is in response to a bug which was pointed out in hanami#106. We added `require 'sassc'`, which turned out to be a breaking change. If a user doesn't have `sassc` listed in their Gemfile, they'll get load errors. In order to resolve this, I'm opting to wrap both `SassC` and `Sass`, with the idea that we can remove the reliance on `Sass` soon (the sass-ruby gem is past end-of-life).

landongrindheim · 2019-09-04T14:25:05Z

Code coverage goes down due to

          require 'sassc'
          ::SassC
        rescue LoadError => exception
          begin
            require 'sass'
            ::Sass
          rescue LoadError
            raise exception
          end

Not sure how to account for the case where sassc is not available (tried stubbing the require call, but it was unsuccessful and felt wrong). Open to any tips on that 🙂

jodosha

@landongrindheim Thanks for this PR. 💚

Did you also try to manage the loading of SASS via Tilt? If so, why did you pick this way and not the other?

NOTE: I'm not suggesting you to do so, just suggesting to explore all the options on the table.

If we decide to adopt this approach, we should port the other compilers with the same approach: wrap into an engine.

landongrindheim · 2019-09-04T18:49:13Z

@jodosha I chose to go this route because Tilt doesn't implement #dependencies (or expose a method which would do that AFAICT).

I suppose I chose this method in order to preserve the dependency caching done when the compiler uses Sass. If we used Tilt, we'd have one fewer direct (gem) dependency, but compilation would take longer when SassC was not available.

Will Tilt rely on one's Gemfile for the presence of the Sass[C] source code? If so, that's a layer of another layer of indirection.

jodosha

@landongrindheim I left inline comments for issues to be fixed. Please remember to extract an engine for Less as well, even if it will use Tilt, but for consistency sake, it's better.

Would you please to take care of this following iteration? Thanks in advance.

lib/hanami/assets/sass/engine.rb

spec/unit/hanami/assets/sass/engine_spec.rb

lib/hanami/assets/sass/engine.rb

Support for Sass was dropped in Assets v 1.3.2. No need to re-introduce.

Since fd4bfcc, the interface for the Sass compiler has been wrapped inside an object we own. In order to be consistent with our compilers, the same approach is being taken here.

jodosha

@landongrindheim Sorry for the back and forth on this.

Given the engine idea was born to abstract sass and sassc, but now that we don't support sass anymore, we can simplify the code, by removing the engine at all.

Please have a look at this diff:

diff --git a/lib/hanami/assets/compilers/sass.rb b/lib/hanami/assets/compilers/sass.rb
index 28cc0ed..3e0f92e 100644
--- a/lib/hanami/assets/compilers/sass.rb
+++ b/lib/hanami/assets/compilers/sass.rb
@@ -1,5 +1,3 @@
-require 'hanami/assets/sass/engine'
-
 module Hanami
   module Assets
     module Compilers
@@ -18,13 +16,20 @@ module Hanami
           name.to_s =~ EXTENSIONS
         end
 
+        # @since 1.3.3
+        # @api private
+        def initialize(*args)
+          super
+          require "sassc"
+        end
+
         private
 
         # @since 0.3.0
         # @api private
         def renderer
           @renderer ||=
-            Hanami::Assets::Sass::Engine.new(
+            ::SassC::Engine.new(
               source.read,
               syntax: target_syntax,
               load_paths: load_paths
@@ -34,7 +39,9 @@ module Hanami
         # @since 0.3.0
         # @api private
         def dependencies
-          renderer.dependencies
+          renderer.dependencies.map(&:filename)
+        rescue ::SassC::NotRenderedError
+          []
         end
 
         # @since 1.3.2

The compiler dynamically loads sassc. It it isn't in the app Gemfile, it crashes and I believe it's self-explanatory: you had a .sass/.scss file, but you haven't bundled sassc gem.

If it doesn't blow up at the initialize time, it's safe to refer to ::SassC constant, the way we use to do right now in master.

That makes both Sass and Less engines obsolete. Sorry again.

Would you like to take care of this? What's the best way to get in touch with you? Thanks.

landongrindheim · 2019-09-12T12:19:36Z

@jodosha I'm glad you were able to look at this from that perspective. Without supporting both SassC and Sass, the engine concept is just indirection.

I'm reachable by @ing me on GitHub, or by email at landon dot grindheim at gmail dot com. It's been fun contributing to Hanami's repos. I hope to do more in the future 😄

landongrindheim · 2019-09-12T14:35:29Z

@jodosha Looks like the build was cancelled. Are you able to retry it?

Edit: I'm going to amend the last commit and force push to trigger the build.

@jodosha

As pointed out by @jodosha, the wrapped engine concept was introduced as a way to deal with Sass *and* SassC being dependencies. Since we've dropped support for Sass, the engines are just a layer of indirection.

jodosha · 2019-09-13T07:49:40Z

@landongrindheim Thanks for your help! I appreciated that!

landongrindheim mentioned this pull request Sep 4, 2019

sassc should be a runtime dependency #106

Closed

Correct Rubocop violations

2c50e26

jodosha self-requested a review September 4, 2019 16:56

jodosha reviewed Sep 4, 2019

View reviewed changes

jodosha requested changes Sep 11, 2019

View reviewed changes

lib/hanami/assets/sass/engine.rb Outdated Show resolved Hide resolved

spec/unit/hanami/assets/sass/engine_spec.rb Outdated Show resolved Hide resolved

lib/hanami/assets/sass/engine.rb Outdated Show resolved Hide resolved

jodosha assigned jodosha and cllns Sep 11, 2019

jodosha added the fix label Sep 11, 2019

jodosha added this to the v1.3.3 milestone Sep 11, 2019

landongrindheim added 3 commits September 11, 2019 08:12

Remove support for Sass

d7e0206

Support for Sass was dropped in Assets v 1.3.2. No need to re-introduce.

Wrap Less engine (Tilt) for consistency

03e6364

Since fd4bfcc, the interface for the Sass compiler has been wrapped inside an object we own. In order to be consistent with our compilers, the same approach is being taken here.

Return empty array when there are no dependencies

c340c57

jodosha requested changes Sep 12, 2019

View reviewed changes

landongrindheim force-pushed the wrap-sass-gems branch from 4d681c0 to 81f598d Compare September 12, 2019 12:21

Back out Sass/Less engine changes

bfb025b

As pointed out by @jodosha, the wrapped engine concept was introduced as a way to deal with Sass *and* SassC being dependencies. Since we've dropped support for Sass, the engines are just a layer of indirection.

landongrindheim force-pushed the wrap-sass-gems branch from 81f598d to bfb025b Compare September 12, 2019 17:58

jodosha merged commit 1754cb0 into hanami:master Sep 13, 2019

jodosha approved these changes Sep 13, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap SassC/Sass gems #108

Wrap SassC/Sass gems #108

landongrindheim commented Sep 4, 2019

landongrindheim commented Sep 4, 2019

jodosha left a comment

landongrindheim commented Sep 4, 2019 •

edited

jodosha left a comment

jodosha left a comment

landongrindheim commented Sep 12, 2019

landongrindheim commented Sep 12, 2019 •

edited

jodosha commented Sep 13, 2019

Wrap SassC/Sass gems #108

Wrap SassC/Sass gems #108

Conversation

landongrindheim commented Sep 4, 2019

landongrindheim commented Sep 4, 2019

jodosha left a comment

Choose a reason for hiding this comment

landongrindheim commented Sep 4, 2019 • edited

jodosha left a comment

Choose a reason for hiding this comment

jodosha left a comment

Choose a reason for hiding this comment

landongrindheim commented Sep 12, 2019

landongrindheim commented Sep 12, 2019 • edited

jodosha commented Sep 13, 2019

landongrindheim commented Sep 4, 2019 •

edited

landongrindheim commented Sep 12, 2019 •

edited